Ignore unknown elements #121

fjolnir · 2018-03-29T06:44:21Z

fjolnir

Thanks for the help! I hope my comments aren't too pedantic 🙇‍♂️

fjolnir · 2018-03-29T06:45:49Z

SVGEngine.mm

    while(xmlTextReaderRead(_xmlReader) == 1) {
        int const type = xmlTextReaderNodeType(_xmlReader);
        const char * const tag = (char *)xmlTextReaderConstName(_xmlReader);

        CGPathRef path = NULL;
-        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "path") == 0)
+        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "svg") == 0) {


Let's insert a comment here along the lines of

if (tag==<svg>) { // Prevent <svg> from being ignored` } else ...```

fjolnir · 2018-03-29T06:46:01Z

SVGEngine.mm

@@ -125,7 +133,9 @@ @interface SVGAttributeSet () {
                pushGroup(readAttributes());
            else if(type == XML_READER_TYPE_END_ELEMENT)
                popGroup();
-        }
+        } else if(type == XML_READER_TYPE_ELEMENT)


Please use curlies {}

Additionally this check should just be if (type == XML_READER_TYPE_ELEMENT && !xmlTextReaderIsEmptyElement(_xmlReader)) skipping the next if statement

Since this is changing to be a single-line branch I presume that you don't want curlies around it, in order to match the rest of the code here?

fjolnir · 2018-03-29T06:47:42Z

SVGEngine.mm

@@ -103,12 +103,20 @@ @interface SVGAttributeSet () {
                                              valueOptions:NSMapTableStrongMemory];
    NSMutableArray * const paths = [NSMutableArray new];

+    unsigned int depthWithinUnknownElement = 0;


This file uses NSUInteger.

fjolnir · 2018-03-29T06:48:54Z

SVGEngine.mm

    while(xmlTextReaderRead(_xmlReader) == 1) {
        int const type = xmlTextReaderNodeType(_xmlReader);
        const char * const tag = (char *)xmlTextReaderConstName(_xmlReader);

        CGPathRef path = NULL;
-        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "path") == 0)
+        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "svg") == 0) {
+        } else if(depthWithinUnknownElement > 0) {


I think it'd be a bit more readable if this check came first.

More correct too, 💯

fjolnir · 2018-03-29T06:52:02Z

I'm not sure if there are any wrapper tags that we wouldn't want to ignore. Best to be careful here.

cysp

Hey, just saw that this PR got raised.
I'll go through and make the changes soon.

cysp · 2018-04-03T01:19:38Z

SVGEngine.mm

    while(xmlTextReaderRead(_xmlReader) == 1) {
        int const type = xmlTextReaderNodeType(_xmlReader);
        const char * const tag = (char *)xmlTextReaderConstName(_xmlReader);

        CGPathRef path = NULL;
-        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "path") == 0)
+        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "svg") == 0) {


cysp · 2018-04-03T01:19:57Z

SVGEngine.mm

    while(xmlTextReaderRead(_xmlReader) == 1) {
        int const type = xmlTextReaderNodeType(_xmlReader);
        const char * const tag = (char *)xmlTextReaderConstName(_xmlReader);

        CGPathRef path = NULL;
-        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "path") == 0)
+        if(type == XML_READER_TYPE_ELEMENT && strcasecmp(tag, "svg") == 0) {
+        } else if(depthWithinUnknownElement > 0) {


More correct too, 💯

cysp · 2018-04-03T01:21:33Z

SVGEngine.mm

@@ -103,12 +103,20 @@ @interface SVGAttributeSet () {
                                              valueOptions:NSMapTableStrongMemory];
    NSMutableArray * const paths = [NSMutableArray new];

+    unsigned int depthWithinUnknownElement = 0;


cysp · 2018-04-04T04:21:45Z

Comments addressed (I think) in a fixup! commit on top, to be squashed prior to merge.
I'm not sure what direction to take from the suggestion to be careful when ignoring elements though?

Of the "container" and "structural" elements, I think the only one that could be correctly handled by rendering its contents into the document would be a, but it can still have styling and transformation attributes on it, so I think that they would have to be handled similarly to g, by pushing and popping the context?

fjolnir · 2018-04-04T07:07:03Z

Changes look good. Could you add || strcasecmp(tag, "a") == 0 to the group check?

arielelkin · 2018-04-04T15:10:40Z

Just a quick headsup: we now have a CHANGELOG to make it easier to keep track of new features and bugs, and to credit our contributors. It'd be great if your PR also updates it.

cysp · 2018-04-05T00:07:53Z

A test suite too! I'll make up some cases for this change.

cysp · 2018-04-05T02:11:37Z

The tests are pretty cheesy but they do test the functionality. :-)
I tried to use a transform in the "respects attributes on <a> elements" test but it seems that the svg string representation code doesn't deal with that (it blows up on non-string, non-CGColorRef attribute values) and fixing that turned into too much change to throw into this PR.

cysp · 2018-04-05T02:13:19Z

Also it looks like the ordering of attributes in the produced SVG string is not fixed? The ordering does seem deterministic, as the tests pass once the magic ordering for that testcase is determined, but it's not predictable?

fjolnir · 2018-04-05T02:14:19Z

The order is undefined.

cysp · 2018-04-05T02:21:59Z

Hm, indeed.
Looks like the existing tests (and now these) were just getting lucky when iterating over the NSDictionary.
Fortunately these tests can fairly easily be changed to just inspect the attributes set on each path rather than implicitly doing that by testing the string representation.

fjolnir · 2018-04-05T02:24:16Z

You could x=parse(svg) and check that parse(generateSvg(x)) == x

arielelkin · 2018-04-05T12:59:26Z

PocketSVGTests/PocketSVGTests.swift

+        let paths = SVGBezierPath.paths(fromSVGString: svgString)
+        XCTAssertEqual(paths.count, 1)
+
+        guard let path = paths.first else {


if XCTAssertEqual(paths.count, 1) passes, then we know that paths.first != nil, so this guard let is redundant and it's safe to do path!.bounds below.

I figured that bailing after failing the assertion was friendlier than hitting a fatal error and breaking the whole test run in the case that we don't parse any paths out of the svg?

an individual test's execution stops as soon as an assertion fails, so if we don't parse any paths out of the svg we'll duly fail the assertion on line 101 and we're guaranteed not to execute the code with the force unwrap.

Hm. That’s not how Xcode (9.3) behaves on my computer and I can’t see .continueAfterFailure being set anywhere. Is that a global configuration option in Xcode?

ah, sorry, you're absolutely right, I hadn't set .continueAfterFailure to false

I have now: 1c014fa

arielelkin · 2018-04-05T13:01:21Z

PocketSVGTests/PocketSVGTests.swift

+        let paths = SVGBezierPath.paths(fromSVGString: svgString)
+        XCTAssertEqual(paths.count, 1)
+
+        guard let path = paths.first else {


ditto as above.

arielelkin · 2018-04-05T13:23:18Z

@fjolnir

You could x=parse(svg) and check that parse(generateSvg(x)) == x

If ordering of attributes is undefined, this could fail even if parsing and generation are done properly, no?

arielelkin · 2018-04-05T13:42:46Z

Fortunately these tests can fairly easily be changed to just inspect the attributes set on each path rather than implicitly doing that by testing the string representation.

Testing that the SVGAttributes are correctly set is another test (which I've now added). But what I wanted to test was that the string representation is correctly generated.

I thought of doing something like:

XCTAssert(bezierPath.svgRepresentation.contains("<path stroke=\"black\""))

but if attributes can be in order, then this test will unduly fail if we generated <path fill=\"transparent\" stroke=\"black\"

So we could simplify the tests to this extent:

XCTAssert(bezierPath.svgRepresentation.contains("stroke=\"black\""))

Do you find that'd be at all useful ?

cysp · 2018-04-05T22:18:05Z

Oh, I meant specifically these tests for this functionality being easily changed to just test the attributes. I think the existing string representation tests certainly do have a place and IMO they're most useful remaining as holistic document tests, as long as the contents of the documents can be given a stable ordering.

arielelkin · 2018-04-17T10:47:30Z

@cysp @fjolnir

Please let me know if there are any other changes you'd like to make to this PR, as I'd like to merge it within a couple of days. I can take care of removing the guard let (see #121 (review)).

arielelkin · 2018-06-25T12:48:55Z

merged via fc4c0af

fjolnir commented Mar 29, 2018

View reviewed changes

cysp reviewed Apr 3, 2018

View reviewed changes

cysp force-pushed the feature/ignore-unknown-elements branch from 02e2c31 to cf9525a Compare April 4, 2018 04:07

cysp force-pushed the feature/ignore-unknown-elements branch from cf9525a to 264ed73 Compare April 4, 2018 07:46

cysp added 4 commits April 5, 2018 11:12

Ignore unknown elements

88be637

Treat a elements like g in terms of respecting attributes

4a1ce47

Update changelog

796b456

Type the return value of +[SVGBezierPath pathsFromSVGString:]

0e89905

cysp force-pushed the feature/ignore-unknown-elements branch from 264ed73 to d1976bb Compare April 5, 2018 02:09

cysp force-pushed the feature/ignore-unknown-elements branch from d1976bb to 9ca0d9c Compare April 5, 2018 02:39

cysp added 2 commits April 5, 2018 12:54

Test that mask elements are ignored

a5127fa

Test that attributes are respected on a elements

87e6b71

cysp force-pushed the feature/ignore-unknown-elements branch from 9ca0d9c to 87e6b71 Compare April 5, 2018 02:55

arielelkin requested changes Apr 5, 2018

View reviewed changes

arielelkin closed this Jun 25, 2018

arielelkin mentioned this pull request Jun 25, 2018

Unknown elements' children are treated the same as direct children of the svg element #118

Closed

Ignore unknown elements #121

Ignore unknown elements #121

Conversation

fjolnir commented Mar 29, 2018

fjolnir left a comment

Choose a reason for hiding this comment

fjolnir Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjolnir commented Mar 29, 2018

cysp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cysp commented Apr 4, 2018

fjolnir commented Apr 4, 2018

arielelkin commented Apr 4, 2018

cysp commented Apr 5, 2018

cysp commented Apr 5, 2018

cysp commented Apr 5, 2018

fjolnir commented Apr 5, 2018

cysp commented Apr 5, 2018

fjolnir commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielelkin Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielelkin commented Apr 5, 2018

arielelkin commented Apr 5, 2018

cysp commented Apr 5, 2018

arielelkin commented Apr 17, 2018

arielelkin commented Jun 25, 2018

fjolnir Mar 29, 2018 •

edited

Loading

arielelkin Apr 5, 2018 •

edited

Loading